Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lap Integrity Check with previous lap "Time" #449

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

d-tomasino
Copy link
Contributor

Minor issue that came up during the discussion in #404. The control flags used in core.py apparently did not include checking the previous lap, but did include checking the integrity of the lap based on the sum of the 3 sectors against the corresponding "LapTime". I added the previous lap check mentioned earlier (apparently now it seems to mention integrity errors with the code above). If anything is not exact I am open to suggestions and/or improvements

After this possible implementation we could focus on the red flag bug issue

fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Outdated Show resolved Hide resolved
fastf1/core.py Show resolved Hide resolved
fastf1/core.py Show resolved Hide resolved
@theOehrly
Copy link
Owner

Ignore test run failures for now. They are currently unrelated.

@theOehrly
Copy link
Owner

Sorry that it took me so long to respond. I was busy and also travelling a bit. And this was not something I could just review on my phone 😅

Basically everything I criticized in the review is readability related. This is not completely on you, this block of code with multi-stage checks is already not very readable the way it is now. But it took me quite a while to understand what's going on with your additional 'flags' mixed into the other steps. Making the additional check self-contained should increase readability quite a bit.

@d-tomasino
Copy link
Contributor Author

Good morning!!! Thank you for your precious suggestions.
In the last commit I followed your ideas trying to keep check_4 self-contained. I only left two conversations unresolved to see what you think and suggested a personal improvement to one of them. Let me know your thoughts about 😄

@theOehrly
Copy link
Owner

theOehrly commented Oct 9, 2023

That looks much more readable already. I made one more suggestion regarding the duplicate integrity error for a single lap. But after that, I think this is good to merge.

Edit: line length needs to be fixed though

@d-tomasino
Copy link
Contributor Author

Good morning! I've resolved the conversations and fixed the line length of the code i wrote. Please note for the future that other lines of core.py needs to be fixed for line length, too.

For any other issues please let me know. Thank you!

@theOehrly
Copy link
Owner

Everything looks good now, thank you!

Please note for the future that other lines of core.py needs to be fixed for line length, too.

Yes, that's why the line length rule is currently only enforced on changes, so that the overall code base slowly moves towards proper line length. That's less intrusive than doing one huge commit that changes hundreds or thousands of lines throughout all files.

@theOehrly theOehrly merged commit 25339e1 into theOehrly:master Oct 17, 2023
7 checks passed
Casper-Guo pushed a commit to Casper-Guo/Fast-F1 that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants